feat: Revamp wait for animation to end tool + tiny fixes #92
Conversation
…unction using time.sleep
…it_for_delay tool
…ait_for_delay tool
WalkthroughRemoved tracking and use of screenshot Base64 across state, agents, and SDK; replaced the animation-wait mechanism with a millisecond delay API and added a wait-for-delay tool and wrapper. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent / LLM
participant Wrapper as wait_for_delay_wrapper
participant Tool as wait_for_delay (tool)
participant Controller as wait_for_delay_controller
participant Graph as Graph State
Note over Wrapper,Tool: wait_for_delay tool flow
Agent->>Wrapper: request wait_for_delay(time_in_ms)
Wrapper->>Tool: invoke wait_for_delay(tool_call_id, state, agent_thought, time_in_ms)
Tool->>Controller: run wait_for_delay_controller(time_in_ms) (async/thread)
alt success
Controller-->>Tool: success
Tool->>Graph: update EXECUTOR_MESSAGES_KEY and agent_thoughts (success)
Tool-->>Wrapper: success result
Wrapper-->>Agent: "Successfully waited for X milliseconds."
else failure
Controller-->>Tool: error
Tool->>Graph: update EXECUTOR_MESSAGES_KEY and agent_thoughts (failure)
Tool-->>Wrapper: failure result
Wrapper-->>Agent: "Failed to wait for delay."
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
minitap/mobile_use/tools/mobile/wait_for_delay.py (1)
40-42: Add upper bound validation and document the default value.The validation sets negative values to 1000ms, but this isn't mentioned in the docstring. Additionally, there's no upper bound check here (though this should ideally be enforced in the controller at lines 558-561 of
minitap/mobile_use/controllers/mobile_command_controller.py).Apply this diff to add an upper bound and improve clarity:
+ # Validate and clamp time_in_ms to reasonable bounds if time_in_ms < 0: time_in_ms = 1000 + elif time_in_ms > 10000: + time_in_ms = 10000 output = wait_for_delay_controller(time_in_ms)Also update the docstring to document this behavior:
Args: - time_in_ms: The number of milliseconds to wait. + time_in_ms: The number of milliseconds to wait (negative values default to 1000ms, capped at 10000ms).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
minitap/mobile_use/agents/contextor/contextor.py(0 hunks)minitap/mobile_use/agents/cortex/cortex.py(0 hunks)minitap/mobile_use/controllers/mobile_command_controller.py(3 hunks)minitap/mobile_use/graph/state.py(0 hunks)minitap/mobile_use/sdk/agent.py(0 hunks)minitap/mobile_use/tools/index.py(2 hunks)minitap/mobile_use/tools/mobile/wait_for_delay.py(1 hunks)
💤 Files with no reviewable changes (4)
- minitap/mobile_use/graph/state.py
- minitap/mobile_use/agents/cortex/cortex.py
- minitap/mobile_use/agents/contextor/contextor.py
- minitap/mobile_use/sdk/agent.py
🧰 Additional context used
🧬 Code graph analysis (3)
minitap/mobile_use/tools/index.py (2)
minitap/mobile_use/controllers/mobile_command_controller.py (1)
wait_for_delay(558-561)minitap/mobile_use/tools/mobile/wait_for_delay.py (1)
wait_for_delay(20-64)
minitap/mobile_use/tools/mobile/wait_for_delay.py (4)
minitap/mobile_use/context.py (1)
MobileUseContext(57-74)minitap/mobile_use/controllers/mobile_command_controller.py (1)
wait_for_delay(558-561)minitap/mobile_use/graph/state.py (2)
State(20-88)asanitize_update(64-88)minitap/mobile_use/tools/tool_wrapper.py (1)
ToolWrapper(9-12)
minitap/mobile_use/controllers/mobile_command_controller.py (1)
minitap/mobile_use/tools/mobile/wait_for_delay.py (1)
wait_for_delay(20-64)
🔇 Additional comments (3)
minitap/mobile_use/tools/index.py (1)
15-17: LGTM! Clean migration to the delay-based tool.The import and reference changes correctly reflect the transition from animation-based waiting to a simple delay mechanism.
Also applies to: 32-32
minitap/mobile_use/tools/mobile/wait_for_delay.py (1)
42-48: Note: Error handling path is currently unreachable.The controller function
wait_for_delay_controller(defined at lines 558-561 inmobile_command_controller.py) always returnsNoneon success and doesn't catch exceptions. This meanshas_failedwill always beFalse, and the failure handling path (on_failure_fn) is never executed. If the sleep operation were to fail (unlikely but possible with system interrupts), it would raise an exception rather than returning an error value.This is likely acceptable for the current implementation, but if you want robust error handling, consider wrapping the controller call in a try-except:
try: output = wait_for_delay_controller(time_in_ms) has_failed = False except Exception as e: output = str(e) has_failed = TrueOtherwise, you might simplify the code by removing the unused error handling:
output = wait_for_delay_controller(time_in_ms) agent_outcome = wait_for_delay_wrapper.on_success_fn(time_in_ms) tool_message = ToolMessage( tool_call_id=tool_call_id, content=agent_outcome, status="success", )minitap/mobile_use/controllers/mobile_command_controller.py (1)
564-572: I need to gather more context about the PR changes and the intent behind animation-based waiting. Let me check if there are any other relevant files or commits that explain the migration.The review comment is based on incomplete code inspection and should be disregarded.
The original concern states the function "still appends a
waitForAnimationToEndcommand with a hardcoded 500ms timeout," but this is inaccurate. The function appends this command only when explicitly requested via thewait_for_animation_to_endparameter (which defaults toFalse).Of the 6 call sites identified:
- launch_app (line 502-504) explicitly passes
wait_for_animation_to_end=True- All other callers (long_press, stop_app, open_link, back, press_key) rely on the default behavior without animation waiting
The Maestro documentation recommends avoiding fixed waits and reserving
waitForAnimationToEndfor specific cases, making the conditional approach inlaunch_appappropriate for that operation. The 500ms timeout is reasonable within Maestro's design model.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
minitap/mobile_use/tools/mobile/wait_for_delay.py (1)
20-20: Add return type annotation to match ToolWrapper expectation.ToolWrapper.tool_fn_getter expects Callable[[MobileUseContext], BaseTool]. Annotate and import BaseTool.
-from langchain_core.tools import tool +from langchain_core.tools import tool, BaseTool @@ -def get_wait_for_delay_tool(ctx: MobileUseContext): +def get_wait_for_delay_tool(ctx: MobileUseContext) -> BaseTool:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
minitap/mobile_use/tools/mobile/wait_for_delay.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
minitap/mobile_use/tools/mobile/wait_for_delay.py (4)
minitap/mobile_use/context.py (1)
MobileUseContext(57-74)minitap/mobile_use/controllers/mobile_command_controller.py (1)
wait_for_delay(558-561)minitap/mobile_use/graph/state.py (2)
State(20-88)asanitize_update(64-88)minitap/mobile_use/tools/tool_wrapper.py (1)
ToolWrapper(9-12)
🔇 Additional comments (2)
minitap/mobile_use/tools/mobile/wait_for_delay.py (2)
61-69: ****The review comment is based on a misunderstanding of the
AgentNodetype. AgentNode is defined asLiteral["planner", "orchestrator", "cortex", "screen_analyzer", "executor"], not an enum class with attributes likeAgentNode.executor.Since AgentNode is a Literal type of strings, passing
agent="executor"is correct and fully type-compatible with the function signatureagent: AgentNode | None = None. The code in wait_for_delay.py at lines 61-69 follows the same pattern as all other callers in the codebase and requires no changes.Likely an incorrect or invalid review comment.
54-59: Status parameter is supported in langchain-core 0.3.75—review comment is incorrect.The project requires
langchain-core>=0.3.75, which supports thestatusparameter onToolMessage. The current code inwait_for_delay.py(lines 54–59) correctly usesstatus="error" if has_failed else "success", and this is consistent with other files in the codebase (e.g.,launch_app.py,tool_node.py). Movingstatustoadditional_kwargswould contradict the supported API and is unnecessary.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
minitap/mobile_use/tools/mobile/wait_for_delay.py (1)
44-47: Document or reconsider the negative-value default.Negative time values are silently coerced to 1000ms, which is arbitrary and undocumented in the docstring. Consider either:
- Raising a
ValueErrorfor negative inputs to catch caller bugs, or- Defaulting to 0ms instead of 1000ms for more predictable behavior
Update the docstring to document whichever behavior you choose.
Example if raising an error:
- if time_in_ms < 0: - time_in_ms = 1000 + if time_in_ms < 0: + raise ValueError(f"time_in_ms must be non-negative, got {time_in_ms}")Or if defaulting to 0:
- if time_in_ms < 0: - time_in_ms = 1000 + if time_in_ms < 0: + time_in_ms = 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
minitap/mobile_use/config.py(1 hunks)minitap/mobile_use/tools/mobile/wait_for_delay.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
minitap/mobile_use/config.py (1)
minitap/mobile_use/utils/logger.py (1)
warning(85-86)
minitap/mobile_use/tools/mobile/wait_for_delay.py (4)
minitap/mobile_use/context.py (1)
MobileUseContext(57-74)minitap/mobile_use/controllers/mobile_command_controller.py (1)
wait_for_delay(558-561)minitap/mobile_use/graph/state.py (2)
State(20-88)asanitize_update(64-88)minitap/mobile_use/tools/tool_wrapper.py (1)
ToolWrapper(9-12)
🔇 Additional comments (4)
minitap/mobile_use/config.py (1)
246-265: LGTM! Solid validation and diagnostic improvements.The refactored merge logic correctly validates override keys against the base configuration and provides clear diagnostic messages with full path tracking. The recursive approach properly handles nested dictionaries while ignoring unsupported keys.
minitap/mobile_use/tools/mobile/wait_for_delay.py (3)
1-18: LGTM! Clean imports and reasonable delay cap.The imports are appropriate for the async tool implementation, and the 60-second maximum delay cap prevents excessive blocking.
48-54: LGTM! Properly offloads blocking operation.The use of
asyncio.to_threadcorrectly prevents blocking the event loop, addressing the previous review concern. Exception handling is appropriate.
55-84: LGTM! Correct state update and wrapper integration.The tool correctly:
- Constructs agent outcomes using the wrapper's callback functions
- Creates a
ToolMessagewith appropriate status and error details- Updates state through
asanitize_updatewith the executor agent context- Configures the wrapper with matching lambda signatures
🚀 What's new?
Revamping the wait for animation to end tool with a somple wait for dealy (sleep).
Tiny fixes to improve mobile-use execution.
🤔 Type of Change
What kind of change is this? Mark with an
x✅ Checklist
Before you submit, please make sure you've done the following. If you have any questions, we're here to help!
ruff check .andruff format .pass).💬 Any questions or comments?
Have a question or need some help? Join us on Discord!
Summary by CodeRabbit
Removed Features
New Features
Breaking Changes